-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add guardrails for headed mode #1197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add guardrails for headed mode #1197
Conversation
| } | ||
| } | ||
| } catch { | ||
| // Config loading errors are handled by existing playwrightConfigPath validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you verify this? I don't think this is 100% true, if we fail to load we should throw an error that we are unable to parse the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ferrandiaz Thanks for the review, you are right. Should be fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to check if we can avoid loading the config file multiple times. Maybe not but it seems wasteful.
Just blocking the merge for a moment.
| const playwrightConfig = await Session.loadFile<any>(this.playwrightConfigPath) | ||
|
|
||
| if (playwrightConfig?.use?.headless === false) { | ||
| diagnostics.add(new UnsupportedPropertyDiagnostic( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only false is not supported, use InvalidPropertyValueDiagnostic. If any value is unsupported, then use UnsupportedPropertyDiagnostic. It will slightly change the error message, so adjust tests accordingly if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sorccu You are correct, the property is supported with value: true. Changing it to InvalidPropertyValueDiagnostic.
Affected Components
Notes for the Reviewer
Detect if theres a headless: false in the playwright.config and show an error.